SubscriberBlackboxVerificationRules explained #362
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@reactive-streams/contributors I'm trying to wrap some other RS things up for the 1.0.1 rel3ease, and I'd love to include this, if someone would have a slot to review this that'd be just great. 😄 |
|
On a quick browse, this looks OK. Although the use of "verifies" still bothers me. "Checks" would be more appropriate. |
|
@DougLea It's "verifies" as in "tests" and not as in "formal verification" :) |
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
ktoso
added a commit
to ktoso/reactive-streams
that referenced
this pull request
May 29, 2017
…any other signal
ktoso
reviewed
May 29, 2017
There was a problem hiding this comment.
Very good initiative.
I fixed the issues in the TCK pointed out in the notes in these PRs: #375 #374 #373 #372
Please review those (thanks @akarnokd for already doing so).
This PR only needs some minor rewordings, the rest is LGTM.
I propose we merge this, and then merge my PRs one by one and in each of the PR's it will remove the Note that explains the issue.
Please assist in merging @viktorklang.
I expect to finish all other reviews and other things hopefully tonight... but we'll see
| * <li>if the {@code Subscriber} requires external stimulus to begin requesting; override the | ||
| * {@link SubscriberBlackboxVerification#triggerRequest(org.reactivestreams.Subscriber)} method | ||
| * in this case,</li> | ||
| * <li>the {@code TestEnvironment} has large enough timeout specified in case the {@code Subscriber} has some time-delay behavior,</li> |
| * <li>This test doesn't signal {@code onComplete} after the first set of {@code onNext} signals | ||
| * has been emitted and may cause resource leak in | ||
| * {@code Subscriber}s that expect a finite {@code Publisher}.</li> | ||
| * <li>The test ignores cancellation from the {@code Subscriber} and emits the requested amount regardless.</li> |
There was a problem hiding this comment.
Perhaps mention that which is allowed under https://github.com/reactive-streams/reactive-streams-jvm#1.8
| * <p> | ||
| * Notes: | ||
| * <ul> | ||
| * <li>The test checks for the presensce of method named "onComplete" in the current stacktrace when handling |
| * <li>This test emits the number of items requested thus the {@code Subscriber} implementation | ||
| * should not request too much.</li> | ||
| * <li>Only the very first {@code request} amount is considered.</li> | ||
| * <li>This test doesn't signal {@code onComplete} after the first set of {@code onNext} signals |
There was a problem hiding this comment.
Good catch!
Let's just fix that, it's an omission; PR here: #372
| * <p> | ||
| * Notes: | ||
| * <ul> | ||
| * <li>The test checks for the presensce of method named "onError" in the current stacktrace when handling |
| * <p> | ||
| * Notes: | ||
| * <ul> | ||
| * <li>The test ignores cancellation from the {@code Subscriber}.</li> |
There was a problem hiding this comment.
again, note that this is ok, as it may simulate a batching async publisher;
such note: which is allowed under https://github.com/reactive-streams/reactive-streams-jvm#1.8?
| * <ul> | ||
| * <li>Tests {@link #required_spec209_blackbox_mustBePreparedToReceiveAnOnCompleteSignalWithPrecedingRequestCall() §2.9} | ||
| * and {@link #required_spec210_blackbox_mustBePreparedToReceiveAnOnErrorSignalWithPrecedingRequestCall() §2.10} are | ||
| * supposed to cover this case from the {@code Subscriber's} perspective.</li> |
| * <p> | ||
| * Notes: | ||
| * <ul> | ||
| * <li>This could be tested with a synchronous source currently not available within the TCK.</li> |
There was a problem hiding this comment.
let's not sneak in non-spec wording, so with a "synchronous publisher".
We could roll-one in place for this test, feel free to PR if you want.
| * Notes: | ||
| * <ul> | ||
| * <li>Despite the method name, the test doesn't expect a request signal from {@code Subscriber} and emits the | ||
| * {@code onError} signal anyway. |
There was a problem hiding this comment.
whoops, that's a mistake then.
Addressed it in: #374
We (attempt to) cover both cases now, without explicitly waiting for the request which could mean we issue before the request happens, and with explicitly waiting, which means it always is after the request signal. We use the TCK infra to force a request be made.
| * <p> | ||
| * Notes: | ||
| * <ul> | ||
| * <li>Currently, the test doesn't call {@code onSubscribe} on the {@code Subscriber} which violates §1.9. |
There was a problem hiding this comment.
Ouch, good catch - must have remained since days where this was legal.
|
Works either way for me, perhaps easier to get this in and amend as we go along merging the fixes. |
I agree with this and the suggested wording changes as well. |
|
@ktoso You mean you want me to merge this as is, now? (just making sure) |
|
I'm ok with that, and then we continue swiftly merging the follow ups. I'd tag them as pick-next but lack super powers :) |
|
@ktoso Ok. I think we need to do an RC of 1.0.1 to make sure that all currently passing impls have a chance to check if the new TCK works with existing, passing, TCK. Do you agree? |
|
Yes, RCs are likely a good idea since we fix a bunch of things here and there in the TCK, it also gives us time in case something is wrong to fix it. |
|
@ktoso Agreed. Would be goo to give all @reactive-streams/contributors and community a couple of weeks to try it out and report back before committing to the final 1.0.1 |
viktorklang
added a commit
that referenced
this pull request
Jun 16, 2017
=tck #362 signal onComplete in 201 blackbox verification
viktorklang
added a commit
that referenced
this pull request
Jun 16, 2017
+tck #362 complete subscriber under test once done in 205
viktorklang
added a commit
that referenced
this pull request
Jun 16, 2017
=tck #362 blackbox 209 must issue onSubscribe before any other signal
viktorklang
added a commit
that referenced
this pull request
Jun 16, 2017
+tck #362 wait for request signal in 209, and new additional tests
akarnokd
added a commit
to akarnokd/reactive-streams
that referenced
this pull request
Nov 3, 2017
* Repairs formatting issue of tables in spec README * Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException` be thrown. Updates the TCK, Spec and example implementations. * Fixes reactive-streams#211 by clarifying * Fixes reactive-streams#210 by removing 1.12 and repurposing its TCK checks for 1.09 * Clarifies the signalling sequence in the spec and adds TCK verification to ensure signal ordering is proper, also amends the examples to reflect the spec change. * Publish 1.0.0.RC2 fix reactive-streams#215 * Fixes reactive-streams#217 by including the examples project in the publish task * =tck minor test name fixup, it is a required test * fix reactive-streams#212 issue on spec 213 testing wrt Processor * RC3 release /w reactive-streams#222 fix * remove rule 1:12 (produce same elements to all Subscribers) This rule is in conflict with 1:11 which allows a Publisher to treat multiple Subscribers as either as unicast or multicast recipients. The verification of proper multicast behavior (which 1:12 specified) has been retained, the test methods renamed accordingly. * fix three left-over references to deleted rule 1:12 * Fixed wrong footnote reference in README.md * Addresses a couple of typos in the examples for AsyncSubscriber and SyncSubscriber * !TCK clarify what error publisher is + add better readme on what this method is + add better javadoc on this method - removes reference to old style spec annotation from readme + proposing to change method name to "createFailed..." as it is the wording used in the spec and reactive manifesto (footnote 1.1) + more info in tck/README that it is not legal to signal on* before sub Resolves reactive-streams#237, reactive-streams#235 * +tck reactive-streams#236 example subscriber whitebox tested, and whitebox fixed * add space to javadoc * +TCK verifyNoAsyncErrors now by default waits, fixes spec111 Resolves reactive-streams#239 * =tck general tck/readme.md cleanup so it matches current code / spec Resolves reactive-streams#99 Depends on reactive-streams#241 * Addresses PR review comments for reactive-streams#246 * Update CopyrightWaivers.txt * +tck explains createElement in more useful terms resolves reactive-streams#231 * +tck reactive-streams#232 explain which tests are mendatory to be "compliant" * Update SubscriberWhiteboxVerification.java Fixes Javadoc generation on Java8+ by having to manually qualify nested classes. * Fixes reactive-streams#233 by implementing support for triggered demand in in the SubscriberBlackboxVerification * Travis PR validation using both JDK 6 and 8 By validating on both JDKs we know the project even builds on 8, while not using features (classes) from JDK8 - so it's still usable for JDK6 projects. Resolves reactive-streams#254 * Small touchups to the TCK README.md * Release 1.0.0.RC4 * Cancel the subscription after receiving all of the pertinent emissions (reactive-streams#259). * Test that 'required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue' completes in a timely manner for fully synchronous publishers (reactive-streams#259). * =tck untested spec308 rule method name adjusted * -tck rm undocumented and unused publisherReferenceGCTimeoutMillis method * update version to 1.0.0.RC5 * Updating documentation to reflect the current version: RC5 * update ref to 1.0.0.final * change 1.0.0.final to 1.0.0 and make sure OSGI manifest has the bundle version * OSGI fix * OSGI fix... * Disambiguate "processing elements" The document generally refers to "elements" as objects traversing a stream. I initially considered simply editing "processing elements" to read "processing components", but there's a section devoted to the definition of this, so better to link them. * Added per request of @viktorklang in reactive-streams#269 * add CC0 label to README * =tck reactive-streams#279 improve completion latch error message * Rename SyncSybscriber.foreach to whenNext * Update README.md Spelling of the company name is Red Hat, not RedHat. * I hereby represent [...] public domain [...] entirety of my contributions. Requested by @viktorklang. * Log test output events to the console * Remove "preview" qualifier from README. * Unbreaks TravisCI OpenJDK6 hostname too long crash * Second attempt at unbreaking the Travis build * Third attempt at fixing the Travis builds * +tck reactive-streams#308 allow configuring "no events during N time" separately * Update to Gradle 2.12 * Reintegrate dangling footnote in Publisher section. - integrate the footnote in rule 1.9 - sign the Copyright Statement * Asynchronous vs Synchronous Processing: reword "push-based stream" * =tck fixes minor misalignment between code and comment, found via .NET port Semantics remain exacly the same, the error we're testing here is about signaling one more element if request comes in again (which we'll do anyway, regardless of status of this flag) * adjust Subscription.cancel javadoc because cancel command does not have to be called asynchronously * Updating Typesafe to Lightbend * Fix a typo in org.reactivestreams.example.unicast.AsyncSubscriber * Add @seratch to CopyrightWaivers.txt * Fixes reactive-streams#333 by adding license headers to /examples/* * Adds a Glossary, Intent-sections and harmonizes verbiage * Clarifying that object equality is a.equals(b) in Intent for 2.12 * add license header to API directory * add license header to TCK * Fix missing cancel() from in tests that don't consume the entire source * Run with default TestEnvironment settings. * Update CopyrightWaivers.txt * =build reactive-streams#349 equal osgi manifest version as real version To have a tangible PR to talk about. Probably enough to resolve reactive-streams#349 Would be followed up with change to 1.0.1 eventually. * Add Javadoc explanation to the TCK test methods about what they do * Don't import org.reactivestreams.tck.TestEnvironment * Fix missing Javadoc tags * TCK: Request -1 in 309 instead of a random non-positive number * Remove the Random instance as well. * Keep the randomness. * Fixing typos in README.md * Minor rewording of 2.6 to make it easier to understand. (reactive-streams#342) * Minor rewording of 2.6 to make it easier to understand. * Fix spelling errors and clarify a couple of sentences * extra coordination * Remove vague statements, be more specific in others * Update javadoc based on ktoso's feedback * Use the wording eagery for error publisher test 104 * Address feedback, add links to the rules in the javadoc * SubscriberBlackboxVerificationRules explained * Non-BC for TCK: Corrects a typo in test method from *Compuatation to *Computation * Adding a glossary item for external synchronization * Repointing links to sources in README to current main release * =tck reactive-streams#362 signal onComplete in 201 blackbox verification * +tck reactive-streams#362 complete subscriber under test once done in 205 * +tck reactive-streams#362 wait for request signal in 209, and new additional tests * =tck check isCancelled in 205 blackbox; sample the state sometimes * =tck reactive-streams#362 blackbox 209 must issue onSubscribe before any other signal * Clarifies the meaning of "stochastic" for skipStochasticTests() * add additional test for optional_spec111. * now test verifies https://github.com/reactive-streams/reactive-streams-jvm#1.11 and https://github.com/reactive-streams/reactive-streams-jvm#1.5 for publishers, if they support multiple subscribers. * add delegate to IdentityProcessorVerification. * add tests for optional_spec111_registeredSubscribersMustReceiveOnNextOrOnCompleteSignals. * additional happy and the failure cases. * clear typos and change comments. * add new PublisherVerification for multi-subscribers tests. * removed onSubscribe constructor call. renamed Demand -> CancelableSubscription. * Change subscription remove logic. * add myself to CopyrightWaivers. * fix tests by using proxied subscriber, thanks Viktor for helping push this fix * Be consistent in reference style We use the `#.##` style in referring to rules everywhere, this one ref was using a different style - fixed that. * Switching to consistent use of apostrophe in spec * More apostrophe fixes * add patriknw to CopyrightWaivers * Version 1.0.1 * =spec reactive-streams#384 amend spec to allow not mentioning rule number in exception message * Update README.md * =tck reactive-streams#384 dont check for cause message when checking 3.9 * Updating versions to 1.0.1-RC2 and clarifying changes in RELEASE-NOTES.md * Fix links to "Terminal state" (reactive-streams#389) * Fix links to "Terminal state" * add angelsanz to CopyrightWaivers.txt * Preparing 1.0.1 (reactive-streams#390) * Bridge between Reactive-Streams and JDK 9 Flow API (reactive-streams#296) * Bridge between Reactive-Streams and JDK 9 Flow API * Apply changes based on ktoso's feedback * Use oraclejdk9, resolve build.gradle conflict * Change txt/code to use "Reactive Streams" as designator * NPE to use the updated parameter name. * Rename bridge class, tester class (+javadoc) * Java 9 Flow bridge: add Subscriber converters (reactive-streams#399) * Java 9 Flow bridge: add Subscriber converters * Fix return type javadoc * Example synchronous range Publisher (reactive-streams#395) * Example synchronous range Publisher * Udpated with rule numbers in comments * Mentioning rule 3.9 again in emit() * Move classes to the unicast package. * [WIP] TCK for j.u.c.Flow types "directly" (reactive-streams#398) * Add JDK9 TCK, using adapters * Fixing wrapping and unwrapping of the wrappers themselves. * Renames the converters to "toX" for RS and "toFlowX" for Flow. Fixes so that the dist url for gradle is http iso https (TravisCI bug?) Adds regression test for bridge converters. * fix formatting * cleanup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds textual explanations to the
SubscriberBlackBoxVerificationRulesand also removes the// Verifies rule:comments fromSubscriberBlackBoxVerification.The description contains a Notes: section which could be considered as problems, bugs or opportunities within the TCK tests. I think most of them are spec violations by the test infrastructure itself.
I haven't looked into
SubsriberWhiteBoxVerificationimplementation to see what's different from the black-box version and thus the test need individual explanations or not.